Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn off file-leak-detector if tests are run with Java 21 #4159

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

melissalinkert
Copy link
Member

See discussion in #4158.

Not sure if there is a better way to do this (in particular, to exclude >= 21), but this should at least allow tests to run on Java 21 for now.

@sbesson
Copy link
Member

sbesson commented Mar 4, 2024

Discussed earlier today with @dgault and @melissalinkert, the current proposal offers a good compromise allowing to run the non-regression repository tests on JDK 21 without dropping support for JDK 8 yet and still testing file leaks.
To facilitate debugging and identification of the JDK used in a test run, we might want to update FormatReaderTestFactory.createInstances to also print the Java version as an INFO level statement.

@melissalinkert
Copy link
Member Author

Java version should now be logged each time tests are run.

As suggested by @dgault, fafc56b picks a different file-leak-detector version based on the JDK version being used. The Ant build changes have been reverted so that file-leak-detector remains enabled. That seems to work locally - I can now both compile and run tests on both 8 and 21. Assuming it also works in the next 4 nightly builds in which it's included, that was easier than I expected and worth considering in the context of ome/ome-common-java#72.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and the nightly CI builds have been running on JDK 8, 11, 17 and 21 with this change included. Only remaining stylistic question is whether we should use something similar to ome/ome-codecs#36 i.e. keep the dependency block in the dependencies and manage a new file-leak-detector.version property via profile.

@sbesson sbesson self-requested a review March 7, 2024 16:53
@melissalinkert melissalinkert added this to the 7.3.0 milestone Mar 11, 2024
@sbesson sbesson merged commit 8bcb3c3 into ome:develop Mar 18, 2024
17 checks passed
@melissalinkert melissalinkert deleted the file-leak-detector-java-21 branch September 6, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants